Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wasm Edge #1864

Merged
merged 54 commits into from
Dec 1, 2023
Merged

Wasm Edge #1864

merged 54 commits into from
Dec 1, 2023

Conversation

Harrm
Copy link
Contributor

@Harrm Harrm commented Nov 10, 2023

Referenced issues

Description of the Change

Benefits

Possible Drawbacks

Usage Examples or Tests

Alternate Designs

Copy link
Contributor

@igor-egorov igor-egorov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪

@@ -101,6 +96,7 @@ namespace {
template <auto mf>
wasm::Literal callHostApiFunc(kagome::host_api::HostApi *host_api,
const wasm::LiteralList &arguments) {
std::cout << "Call Host method " << __PRETTY_FUNCTION__ << "\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SL_DEBUG

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still actual

core/runtime/wasm_edge/wrappers.hpp Outdated Show resolved Hide resolved
core/runtime/wasm_edge/wrappers.hpp Outdated Show resolved Hide resolved
core/runtime/wasm_edge/wrappers.hpp Outdated Show resolved Hide resolved
core/runtime/wasm_edge/wrappers.hpp Outdated Show resolved Hide resolved
core/runtime/wasm_edge/module_factory_impl.cpp Outdated Show resolved Hide resolved
core/runtime/wasm_edge/module_factory_impl.cpp Outdated Show resolved Hide resolved
core/runtime/wasm_edge/module_factory_impl.cpp Outdated Show resolved Hide resolved
core/runtime/wasm_edge/register_host_api.hpp Outdated Show resolved Hide resolved
core/runtime/wasm_edge/register_host_api.hpp Outdated Show resolved Hide resolved
core/application/impl/app_configuration_impl.cpp Outdated Show resolved Hide resolved
@@ -9,10 +9,11 @@
#include <tuple>
#include <utility>

#include <fmt/std.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"fmt/std.h" formatter<std::error_code> = "{category}:{enum}" (no message) conflicts with "qtils/error.hpp" formatter<std::error_code> = "{category}:{enum}:{message}".
Should we replace "qtils/error.hpp" formatter with #include <fmt/std.h>?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also conflicts with <libp2p/outcome/outcome.hpp>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qtils/error.hpp looks more descriptive.

* along with root hash storing logic from the trie db component
*/
class TrieStorageBackend : public BufferStorage {
class TrieNodeStorageBackend : public BufferStorage {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(maybe changes related to #1770 from wasmedge pr)
Use BufferStorage directly, or use wrapper instead of inheritance.
#1770 (comment)

Suggested change
class TrieNodeStorageBackend : public BufferStorage {
using TrieNodeStorageBackend = BufferStorage;
struct TrieValueStorageBackend = BufferStorage;
Suggested change
class TrieNodeStorageBackend : public BufferStorage {
struct TrieNodeStorageBackend { std::shared_ptr<BufferStorage> v; };
struct TrieValueStorageBackend { std::shared_ptr<BufferStorage> v; };

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BufferStorage is good simple interface.
To reuse existing BufferStorage implementations for new interface you need to create proxy wrapper class inheriting new interface (new interface calls corresponding BufferStorage) or just reinterpret_cast.
TrieNodeStorageBackend/TrieValueStorageBackend don't add any functions to BufferStorage, so they must not inherit from it and just wrap pointer.

Comment on lines 16 to 17
class TrieStorageBackendImpl : public TrieNodeStorageBackend,
public TrieValueStorageBackend {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this strange diamond inheritance require virtual inheritance?
(Remove this class, use BufferStorage directly)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are just interfaces.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are ambigous/conflicting interfaces.
Without virtual inheritance this class will contain BufferStorage instance twice (for each non-virtual inheriting subclass).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BufferStorage is abstract base class and wont be instantiated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BufferStorage is abstract base class and wont be instantiated.

You can't create BufferStorage variable because it's vtable is empty,
but subclass always contains/stores superclass (struct SubClass { SuperClass superclass; }).

@@ -20,6 +20,7 @@ namespace kagome::storage {
kBlockBody,
kJustification,
kTrieNode,
kTrieValue,
Copy link
Contributor

@turuslan turuslan Nov 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May use same kTrieNode space with prefix (both!)
node_db = MapPrefix{"n", db.getSpace(kTrieNode)}
value_db = MapPrefix{"v", db.getSpace(kTrieNode)}

core/runtime/wasm_edge/module_factory_impl.cpp Outdated Show resolved Hide resolved
core/runtime/wasm_edge/module_factory_impl.cpp Outdated Show resolved Hide resolved
core/runtime/wasm_edge/module_factory_impl.cpp Outdated Show resolved Hide resolved
core/runtime/wasm_edge/module_factory_impl.cpp Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
HunterGate(
URL https://github.com/qdrvm/hunter/archive/refs/tags/v0.23.257-qdrvm11.tar.gz
SHA1 b2a69ae501bdc99006fb86e55930640004468556
URL "https://github.com/qdrvm/hunter/archive/heads/feature/wasm-edge.zip"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hunter with wasm-edge should be released before merge

core/application/app_configuration.hpp Outdated Show resolved Hide resolved
core/injector/CMakeLists.txt Outdated Show resolved Hide resolved
@@ -156,6 +157,16 @@
#include "runtime/runtime_api/impl/session_keys_api.hpp"
#include "runtime/runtime_api/impl/tagged_transaction_queue.hpp"
#include "runtime/runtime_api/impl/transaction_payment_api.hpp"

#if KAGOME_WASM_COMPILER_WASM_EDGE == 1
Copy link
Member

@xDimon xDimon Nov 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or #ifdef

core/runtime/binaryen/memory_impl.cpp Outdated Show resolved Hide resolved
core/runtime/wasm_edge/module_factory_impl.cpp Outdated Show resolved Hide resolved
core/runtime/wasm_edge/module_factory_impl.hpp Outdated Show resolved Hide resolved
void register_host_api(WasmEdge_ModuleInstanceContext *instance) {
BOOST_ASSERT(instance);
// clang-format off
REGISTER_HOST_METHOD(void, ext_allocator_free_version_1, WasmPointer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you extract list of ext_ function to file common for WAVM and WasmEdge?

Copy link
Contributor

@turuslan turuslan Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want it to look like this?

/// host_api_macro.hpp
// no "#pragma once"
HOST_API_MACRO(void, ext_allocator_free_version_1, WasmPointer)
HOST_API_MACRO(int32_t, ext_allocator_malloc_version_1, int32_t)
...

/// wavm.hpp
#define HOST_API_MACRO(...) wavm.add(...)
#include "host_api_macro.hpp"

/// wasmedge.hpp
#define HOST_API_MACRO(...) wasmedge.add(...)
#include "host_api_macro.hpp"

or try to make tuple-like template?

template <auto...> struct TemplateTuple {};
using HostApiTuple = TemplateTuple<
  &HostApi::ext_allocator_free_version_1,
  &HostApi::ext_allocator_malloc_version_1,
  ...
>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll look into it. Maybe in this PR, maybe make an issue.

core/runtime/wasm_edge/wrappers.hpp Show resolved Hide resolved
Comment on lines 16 to 17
class TrieStorageBackendImpl : public TrieNodeStorageBackend,
public TrieValueStorageBackend {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BufferStorage is abstract base class and wont be instantiated.

core/runtime/wasm_edge/module_factory_impl.cpp Outdated Show resolved Hide resolved
common::BufferView loadN(WasmPointer addr, WasmSize n) const override {
BOOST_ASSERT(n > 0);
auto ptr = WasmEdge_MemoryInstanceGetPointer(mem_instance_, addr, n);
BOOST_ASSERT(ptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw (or you will get NullPointerException)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory methods may be called by the host outside of the runtime call, where we catch exceptions, so given that most our code is exception-free I'm not sure it's a good idea to throw here. I think we're gonna need to tackle this problem separately and produce a uniform solution for all WASM executors in kagome.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadN (and all callers) can return outcome.
host api can return outcome or throw with .value(), and host api wrapper function will check outcome or catch exceptions

core/runtime/wasm_edge/register_host_api.hpp Show resolved Hide resolved
@Harrm Harrm marked this pull request as ready for review November 28, 2023 13:18
@Harrm Harrm enabled auto-merge (squash) December 1, 2023 12:07
@Harrm Harrm merged commit ee15e3d into master Dec 1, 2023
10 checks passed
@Harrm Harrm deleted the feature/wasm-edge branch December 1, 2023 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants